-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pointer): move focus in shadow DOM #1038
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Philipp Fritsche <[email protected]>
…DOM in setup(). Removed old custom elements test
Co-authored-by: Philipp Fritsche <[email protected]>
Co-authored-by: Philipp Fritsche <[email protected]>
Co-authored-by: Philipp Fritsche <[email protected]>
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b322220:
|
@Christian24 Interestingly we'll need to apply a workaround both on Jsdom and the browser to handle |
This entire conversation has caused me into looking into focus handling in Shadow DOM in a different light. It is a mess. It seems to work oddly differently everywhere. How do you want to get this consistent? Also do you want to apply the workarounds to the library's tests or in user land? How do you see chances of jsdom implementing A bit of context maybe as well: We use |
That isn't exclusive to this feature. When you work on this library, you get to know a few quirks of browsers and frameworks you might not expect. ;)
It looks as if the behavior in the browser regarding the user interaction is consistent - according to MDN:
On
I filed an issue. But even if it's implemented, I don't think it is relevant enough to be backported. So most users would still use versions without it.
How keyboard navigation handles shadow trees needs to be researched, but there might be no special handling, so our tab order helper would just need to consider all focusable elements no matter if they are in a shadow tree. |
Codecov Report
@@ Coverage Diff @@
## main #1038 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 88 87 -1
Lines 2061 2091 +30
Branches 691 711 +20
=========================================
+ Hits 2061 2091 +30
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Looks good now. Changes on top of the other PR: |
if (effectiveTarget) { | ||
doFocus(effectiveTarget, true, element) | ||
} else { | ||
// This is not consistent across browsers if there is a focusable ancestor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document this and related gotchas in the docs?
What:
Handle shadow hosts when moving focus per
mousedown
.Why:
One step closer to full shadow DOM support #1026
How:
Check for
element.shadowRoot.delegatesFocus
on the ancestors. Iftrue
, find the first focusable element in its shadow tree.Jsdom doesn't implement
delegatesFocus
. Treatundefined
astrue
.Checklist:
Additional information:
Work on top of #1033 - should be rebased once that is merged